Skip to content

Request ASGI attributes passed to Sampler#1762

Merged
srikanthccv merged 7 commits intoopen-telemetry:mainfrom
tammy-baylis-swi:asgi-attributes-available-in-sampler
Apr 28, 2023
Merged

Request ASGI attributes passed to Sampler#1762
srikanthccv merged 7 commits intoopen-telemetry:mainfrom
tammy-baylis-swi:asgi-attributes-available-in-sampler

Conversation

@tammy-baylis-swi
Copy link
Contributor

@tammy-baylis-swi tammy-baylis-swi commented Apr 18, 2023 •
edited
Loading

Description

This adds HTTP server request attributes to the ASGI instrumentation library's start_span call, making them available to otel sampler's should_sample method. A very basic example:

class MySampler(Sampler):
    def should_sample(
        self,
        parent_context: Optional[OtelContext],
        trace_id: int,
        name: str,
        kind: SpanKind = None,
        attributes: Attributes = None,
        links: Sequence["Link"] = None,
        trace_state: "TraceState" = None,
    ) -> "SamplingResult":
        logger.debug("attributes: %s", attributes)

Before this change, attributes is None. After this change, attributes for an instrumented ASGI app would be:

{'http.scheme': 'http', 'http.host': '172.22.0.2:8005', 'net.host.port': 8005, 'http.flavor': '1.1', 'http.target': '/healthcheck', 'http.url': 'http://172.22.0.2:8005/healthcheck', 'http.method': 'GET', 'http.server_name': '0.0.0.0:8005', 'http.user_agent': 'curl/7.87.0', 'net.peer.ip': '172.22.0.1', 'net.peer.port': 55278, 'http.route': '/healthcheck'}

Fixes #1752

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

I have a custom distro that configures the attribute-logging sampler code in the above Description. On my local I run a FastAPI app with opentelemetry-instrument after local install of opentelemetry-instrumentation-asgi==0.39b0.dev and its dependencies. Then I curl the app's /healthcheck route to see stdout logs of the attributes.

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 18, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@tammy-baylis-swi tammy-baylis-swi changed the title (WIP) Request ASGI attributes passed to Sampler Request ASGI attributes passed to Sampler Apr 18, 2023
@tammy-baylis-swi tammy-baylis-swi marked this pull request as ready for review April 18, 2023 17:22
@tammy-baylis-swi tammy-baylis-swi requested a review from a team April 18, 2023 17:22
@shalevr
Copy link
Member

shalevr commented Apr 20, 2023

Could you update the tests? The attributes are changed so the expected attributes in tests should be updated too

@tammy-baylis-swi
Copy link
Contributor Author

tammy-baylis-swi commented Apr 20, 2023

Could you update the tests? The attributes are changed so the expected attributes in tests should be updated too

Hi @shalevr, the final span attributes are the same before and after the code update. The tests in test_asgi_custom_headers.py and test_asgi_middleware.py check the attributes by using get_finished_spans from the TestBase's InMemorySpanExporter, and the tests pass. The code update changes the timing of attribute assignment.

@srikanthccv srikanthccv enabled auto-merge (squash) April 23, 2023 17:24
auto-merge was automatically disabled April 24, 2023 19:04

Head branch was pushed to by a user without write access

@tammy-baylis-swi
Copy link
Contributor Author

tammy-baylis-swi commented Apr 24, 2023

I've added 7d91b86 to fix a failed test in the aiohttp-client instrumentation library:

test_aiohttp_client_integration.py:118: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
		test_aiohttp_client_integration.py:63: in assert_spans 
 self.assertEqual( 
E AssertionError: Lists differ: [('HT[93 chars]45305/test-path#foobar', 'http.status_code': 200})] != [('HT[93 chars]45305/test-path?query=param#foobar', 'http.status_code': 200})] 
E 
E First differing element 0: 
E ('HTT[91 chars]:45305/test-path#foobar', 'http.status_code': 200}) 
E ('HTT[91 chars]:45305/test-path?query=param#foobar', 'http.status_code': 200}) 
E 
E [('HTTP GET', 
E (<StatusCode.UNSET: 0>, None), 
E {'http.method': 'GET', 
E 'http.status_code': 200, 
E - 'http.url': ' http://127.0.0.1:45305/test-path#foobar'})] 
E + 'http.url': ' http://127.0.0.1:45305/test-path?query=param#foobar'})] 
E ? ++++++++++++

This seems to have fixed the instrumentation tests for py37,8,9,10 and pypy3 on Ubuntu 20.04, while the py311 test last failed with a connection timeout. Is this a known and/or transient issue?:

py311-test-opentelemetry-instrumentation run-test-pre: commands[3] | pip install 'opentelemetry-sdk[test] @ git+https://github.com/open-telemetry/opentelemetry-python.git@2387b4465d930b020df79692a8097e1d54b66ec1#egg=opentelemetry-sdk&subdirectory=opentelemetry-sdk'
Collecting opentelemetry-sdk[test]@ git+https://github.com/open-telemetry/opentelemetry-python.git@2387b4465d930b020df79692a8097e1d54b66ec1#egg=opentelemetry-sdk&subdirectory=opentelemetry-sdk
  Cloning https://github.com/open-telemetry/opentelemetry-python.git (to revision 2387b4465d930b020df79692a8097e1d54b66ec1) to /tmp/pip-install-0tijk4_h/opentelemetry-sdk_d056d10a8af0445582060f9b44fffedd
  Running command git clone --filter=blob:none --quiet https://github.com/open-telemetry/opentelemetry-python.git /tmp/pip-install-0tijk4_h/opentelemetry-sdk_d056d10a8af0445582060f9b44fffedd
  fatal: unable to access 'https://github.com/open-telemetry/opentelemetry-python.git/': Failed to connect to github.com port 443: Connection timed out
  fatal: could not fetch f5236d7c1bc1a9b9598b62fd4d4079e39b695540 from promisor remote
  warning: Clone succeeded, but checkout failed.

  ERROR: py311-test-opentelemetry-instrumentation: commands failed

@cheempz
Copy link

cheempz commented Apr 26, 2023

@srikanthccv @shalevr any chance we can get this merged in soon?

@srikanthccv srikanthccv enabled auto-merge (squash) April 28, 2023 15:41
@srikanthccv srikanthccv merged commit f46a6e1 into open-telemetry:main Apr 28, 2023
@tammy-baylis-swi tammy-baylis-swi deleted the asgi-attributes-available-in-sampler branch April 28, 2023 21:12
@tammy-baylis-swi tammy-baylis-swi mentioned this pull request Jul 11, 2023
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shalevr shalevr shalevr approved these changes

+1 more reviewer

@srikanthccv srikanthccv srikanthccv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make requests span attributes available to samplers for ASGI instrumentation

4 participants

@tammy-baylis-swi @shalevr @cheempz @srikanthccv